Remove build qualifier from server's Version #35172

alpar-t · 2018-11-01T17:10:47Z

Build on #35155 ( and contains commits from )

Rename the constant with an automated IDE refactoring - this generates a high number of changes and changed files.
Pass only the numeric part of the version in places like plugin properties so the server doesn't have to deal with qualifier and snapshot.
adds a new substitution to doc tests as the docs now need to know about both.
adds a new String version field to Build to hold the complete version string passed on from the build.
replace Version.getDisplayVersion with Build.CURRENT.getVersion() introduced above
add a new build_version parameter to version. Was not sure this is the way to go, feedback is welcome. The alternative is to have version.number show the build qualifier as well without adding a new build_version. It did use to have -SNAPSHOT in there, but it's not technically a "number". Not that the content of version.number will change in regardless.

With this change elasticsearch greets as:

{
  "name" : "node-0",
  "cluster_name" : "distribution_run",
  "cluster_uuid" : "qvtY3T1bQi-vR4zO4lrpyQ",
  "version" : {
    "number" : "7.0.0",
    "build_flavor" : "default",
    "build_type" : "zip",
    "build_hash" : "26be21b",
    "build_date" : "2018-11-05T13:40:03.172633Z",
    "build_snapshot" : true,
    "build_version" : "7.0.0-alpha1-SNAPSHOT",
    "lucene_version" : "8.0.0",
    "minimum_wire_compatibility_version" : "6.6.0",
    "minimum_index_compatibility_version" : "6.0.0-beta1"
  },
  "tagline" : "You Know, for Search"
}

And in the logs we have:

 version [7.0.0-alpha1-SNAPSHOT] is a pre-release version of Elasticsearch and is not suitable for production

elasticmachine · 2018-11-01T17:10:49Z

Pinging @elastic/es-core-infra

…ier-server

Allow for qualifier

Now that the version no longer carries information about the qualifier, we still need a way to show the "display version" that does have both qualifier and snapshot. With this change we read it from the jar. A subsequent change will switch to useing it instead of `Version.displayVersion`

nik9000

OK! I left a few small things. I like Build.getVersion() - though maybe not the name? Maybe Build.getQualifiedVersion or something. And it should have javadoc. Because we already have Version and it isn't the same thing.

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantTestPlugin.groovy

client/rest-high-level/src/test/java/org/elasticsearch/client/PingAndInfoIT.java

server/src/test/java/org/elasticsearch/node/NodeTests.java

docs/plugins/discovery-azure-classic.asciidoc

docs/reference/cat/plugins.asciidoc

server/src/main/java/org/elasticsearch/Build.java

alpar-t · 2018-11-06T10:35:04Z

Thanks for your review @nik9000 . I made changes as suggested.

…ier-server

nik9000 · 2018-11-06T15:22:52Z

docs/reference/setup/install/check-running.asciidoc

@@ -24,6 +24,7 @@ which should give you a response something like this:
    "build_hash" : "f27399d",
    "build_date" : "2016-03-30T09:51:41.449Z",
    "build_snapshot" : false,
+    "build_version" : "{qualified_version}",


Should the key here be qualified?

I'm not sure. I don't think build_version is right though.

@nik9000
I'm not entirely sure qualified will make things easier to understand either.
Maybe just have this returned as number ? Users will run production releases most of the time, and they'll just see the same information twice if we have a new field.
I will deal with this in a new PR.

nik9000

I left a few super minor things but this LGTM. Good luck on the backport!

nik9000 · 2018-11-06T15:26:20Z

qa/wildfly/src/test/java/org/elasticsearch/wildfly/WildflyIT.java

-                    Build.CURRENT.isSnapshot() ? "-SNAPSHOT" : "");
+                Locale.ROOT,
+                "%s/employees/1",
+                System.getProperty("tests.jboss.home")


I think "home" means something else in this context. Maybe test.wildfly.root?

nik9000 · 2018-11-06T15:27:45Z

server/src/main/java/org/elasticsearch/Build.java

+        }
+    }
+
+    public String getQualifiedVersion() {


Could you add javadoc to this?

nik9000 · 2018-11-06T15:27:59Z

server/src/main/java/org/elasticsearch/Build.java

@@ -235,9 +264,13 @@ public boolean isSnapshot() {
        return isSnapshot;
    }

+    public boolean isProductionRelease() {


nik9000 · 2018-11-06T15:29:25Z

server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java

@@ -102,7 +101,7 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
            final String versionOutput = String.format(
                    Locale.ROOT,
                    "Version: %s, Build: %s/%s/%s/%s, JVM: %s",
-                    Version.displayVersion(Version.CURRENT, Build.CURRENT.isSnapshot()),
+                Build.CURRENT.getQualifiedVersion(),


I think the indentation on this one is off.

nik9000 · 2018-11-06T15:30:34Z

server/src/main/java/org/elasticsearch/node/Node.java

-    static void warnIfPreRelease(final Version version, final boolean isSnapshot, final Logger logger) {
-        if (!version.isRelease() || isSnapshot) {
+    static void warnIfPreRelease(final Build build,final Logger logger) {
+        if (build.isProductionRelease() == false) {


I think maybe this isn't worth being its own method any more. Now that we're testing isProductionRelease instead.

alpar-t · 2018-11-07T05:36:13Z

@nik9000 this is not planned to be back-ported. I have done so with the work leading up here, but don't plan to do it with this one. Do you think we should ?

…ier-server

alpar-t · 2018-11-07T12:32:31Z

After looking more into it, it seems that it was a deliberate decision not to have -SNAPSHOT in version.number so we'll probably want to stick to that.

With this change, `Version` no longer carries information about the qualifier, we still need a way to show the "display version" that does have both qualifier and snapshot. This is now stored by the build and red from `META-INF`.

javanna · 2018-11-22T17:29:01Z

I noticed that when you run ./gradlew assemble the generated artifacts no longer contain the build qualifier, e.g. elasticsearch-7.0.0-SNAPSHOT.zip rather than elasticsearch-7.0.0-alpha1-SNAPSHOT.zip. Not sure if this was done on purpose, just double checking.

alpar-t · 2018-11-23T14:58:42Z

@javanna that was done on purpose. Qualified builds will be tagged as such as needed.

alpar-t added >non-issue :Delivery/Build Build or test infrastructure v7.0.0 labels Nov 1, 2018

alpar-t added the WIP label Nov 1, 2018

alpar-t force-pushed the remove-build-qualifier-server branch from 3b7cd6a to 8bda2da Compare November 1, 2018 17:35

tylersmalley mentioned this pull request Nov 1, 2018

[build] Support version-qualifier flag elastic/kibana#21663

Merged

alpar-t force-pushed the remove-build-qualifier-server branch from dbd1bcf to d0c3224 Compare November 1, 2018 19:50

alpar-t added 7 commits November 2, 2018 06:01

Remove version qualifier from Java.version

16f840b

Set plugin version numeric only

93b7079

Fix docs tests

6b7be57

Fix rebase

541f0cd

Fix server tets

5e949a0

fix packaging tests

ad68da1

Fix tets that rely on ID

fa5d007

alpar-t force-pushed the remove-build-qualifier-server branch from 844de2f to fa5d007 Compare November 2, 2018 04:12

alpar-t added 8 commits November 2, 2018 07:27

fix HLR test

04f6207

Merge remote-tracking branch 'origin/master' into remove-build-qualif…

c784f08

…ier-server

Set version in meta-inf to build version

dabdd05

Fix HLRC tests

c67a067

fix wildfly test

970eff3

Fix packaging tests for unreleased versions

d6abd1a

Fix version parsing sql client

1fd508d

Allow for qualifier

Fix cli version tests

b4db6b9

alpar-t removed the WIP label Nov 4, 2018

alpar-t added 4 commits November 5, 2018 08:52

Fix doc tests

3ce864e

Remove Version.getDisplayName

26be21b

fix isProductionRelease tests

638b846

nik9000 self-requested a review November 5, 2018 17:27

nik9000 requested changes Nov 5, 2018

View reviewed changes

PR review

37c5812

alpar-t added 3 commits November 6, 2018 13:07

Fix doc tests

b14bb39

Merge remote-tracking branch 'origin/master' into remove-build-qualif…

ecbb10f

…ier-server

Merge remote-tracking branch 'origin/master' into remove-build-qualif…

ebce15a

…ier-server

nik9000 reviewed Nov 6, 2018

View reviewed changes

nik9000 approved these changes Nov 6, 2018

View reviewed changes

alpar-t added 2 commits November 7, 2018 07:36

PR review

6955951

Merge remote-tracking branch 'origin/master' into remove-build-qualif…

9769c44

…ier-server

alpar-t merged commit 8a85b2e into elastic:master Nov 7, 2018

alpar-t deleted the remove-build-qualifier-server branch November 7, 2018 12:01

tylersmalley mentioned this pull request Dec 19, 2018

Nodes Info API does not include version qualifier for version #36859

Closed

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove build qualifier from server's Version #35172

Remove build qualifier from server's Version #35172

alpar-t commented Nov 1, 2018 •

edited

Loading

elasticmachine commented Nov 1, 2018

nik9000 left a comment

alpar-t commented Nov 6, 2018

nik9000 Nov 6, 2018

nik9000 Nov 6, 2018

alpar-t Nov 7, 2018

nik9000 left a comment

nik9000 Nov 6, 2018

nik9000 Nov 6, 2018

nik9000 Nov 6, 2018

nik9000 Nov 6, 2018

nik9000 Nov 6, 2018

alpar-t commented Nov 7, 2018

alpar-t commented Nov 7, 2018

javanna commented Nov 22, 2018

alpar-t commented Nov 23, 2018

Remove build qualifier from server's Version #35172

Remove build qualifier from server's Version #35172

Conversation

alpar-t commented Nov 1, 2018 • edited Loading

elasticmachine commented Nov 1, 2018

nik9000 left a comment

Choose a reason for hiding this comment

alpar-t commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented Nov 7, 2018

alpar-t commented Nov 7, 2018

javanna commented Nov 22, 2018

alpar-t commented Nov 23, 2018

alpar-t commented Nov 1, 2018 •

edited

Loading